feat(storage): migrate storage APIs to AmplifyContext - #14874
Conversation
|
osama-rizk
left a comment
There was a problem hiding this comment.
Reviewed the AmplifyContext migration. The mechanical rename hunks are compiler-guarded so I focused on the two runtime seams (resolveCtxArgs back-compat and the resolveServerContext bridge) and the semi-public /internals surface. Strong PR overall — the server-bridge bug (a20993a) is exactly the one worth catching, and the guarding test genuinely exercises it. A few notes below, mostly nits plus one coordination question on the /internals signature break. Nothing blocking for a feature branch.
| */ | ||
| export const getUrl = (input: GetUrlInput) => | ||
| getUrlInternal(Amplify, { | ||
| export const getUrl = (ctx: AmplifyContext, input: GetUrlInput) => |
There was a problem hiding this comment.
These internals/apis/* functions (copy, downloadData, getProperties, getUrl, list, listPaths, remove, uploadData) now take a required ctx with no global fallback — unlike the public S3 APIs, which kept the (input) overload via resolveCtxArgs. They're re-exported from @aws-amplify/storage/internals, which StorageBrowser / ui-react-storage consume, so this is a breaking change on a semi-public surface. Is the asymmetry intentional (internal consumers migrate in lockstep on this branch) or an oversight? If intentional, worth a line in the PR body so the StorageBrowser team isn't surprised.
There was a problem hiding this comment.
Intentional asymmetry. The internals/ surface is consumed by first-party packages (StorageBrowser / ui-react-storage) that migrate in lockstep on this feature branch, so it takes the end-state required-ctx signature directly; the public APIs keep the zero-ctx overload for backward compatibility. Added a note to the PR description so the StorageBrowser team is aware.
| export function remove(...args: any[]) { | ||
| const [ctx, input] = | ||
| resolveCtxArgs<[RemoveInput | RemoveWithPathInput]>(args); | ||
| if ('key' in input) { |
There was a problem hiding this comment.
Both branches of this if/else are identical (return removeInternal(ctx, input)). It was pre-existing dead code, but this PR rewrote these exact lines — good moment to collapse to a single return removeInternal(ctx, input);. Same duplication in the server remove.ts.
There was a problem hiding this comment.
Attempted the collapse in 2b027b4, but TypeScript rejects it: removeInternal is itself overloaded (RemoveInput vs RemoveWithPathInput), and the union argument does not resolve against the overload set without discrimination — so the branches are not dead after all. Kept them and added a one-line comment explaining why the narrowing is required.
|
|
||
| export function copy(input: CopyInput | CopyWithPathInput) { | ||
| return copyInternal(Amplify, input); | ||
| export function copy(...args: any[]) { |
There was a problem hiding this comment.
...args: any[] erases type safety inside the impl — the typed overloads above are the real contract, and resolveCtxArgs<[…]>(args) is the only thing asserting shape at runtime. Acceptable (standard variadic-overload tradeoff, matches the landed auth pattern), but a one-line comment noting 'overloads are the contract; impl is intentionally untyped' would help the next reader. Applies to every client public API using this pattern.
There was a problem hiding this comment.
Addressed in 2b027b4 — added the one-line contract comment ("overloads are the contract; impl is intentionally untyped, shape enforced by resolveCtxArgs") above each variadic impl in all seven public APIs.
| libraryOptions: amplify.libraryOptions, | ||
| fetchAuthSession: options => amplify.Auth.fetchAuthSession(options ?? {}), | ||
| clearCredentials: () => amplify.Auth.clearCredentials(), | ||
| getTokens: options => amplify.Auth.getTokens(options), |
There was a problem hiding this comment.
getTokens passes options straight through, but fetchAuthSession two lines up defaults it (options ?? {}). If Auth.getTokens(undefined) isn't safe this throws on the server path; if it is, the inconsistency is still worth removing. Mirror the ?? {} or confirm getTokens tolerates undefined.
There was a problem hiding this comment.
Safe and deliberate: AmplifyContext.getTokens(options) has a required options parameter (unlike fetchAuthSession(options?)), so typed callers can never pass undefined and no default is needed — the ?? {} on fetchAuthSession exists precisely because of that optional→required bridge. Documented the asymmetry in a comment in 2b027b4.
| } | ||
|
|
||
| const { tokens, identityId } = await fetchAuthSession(); | ||
| const { tokens, identityId } = await ctx.fetchAuthSession({}); |
There was a problem hiding this comment.
ctx.fetchAuthSession({}) passes an explicit {} where the type makes options optional — every other migrated call site writes fetchAuthSession(). Minor consistency nit.
There was a problem hiding this comment.
Addressed in 2b027b4 — now ctx.fetchAuthSession().
| * on the server this is a request-scoped instance from the server adapter. | ||
| */ | ||
| amplify: AmplifyClassV6; | ||
| amplify: AmplifyContext; |
There was a problem hiding this comment.
The doc comment above still says 'On the client this is the global singleton; on the server ... a request-scoped instance' — but the field type just became AmplifyContext, and removing the raw singleton is the whole point of this change. The prose now describes the old world; worth updating.
There was a problem hiding this comment.
Addressed in 2b027b4 — rewrote the doc comment to describe the AmplifyContext (on the client, the global context created by Amplify.configure(); on the server, a context resolved per request).
9b1bb2d to
a20993a
Compare
f01e017 to
9011273
Compare
Thread AmplifyContext explicitly through the storage package instead of relying on the global Amplify singleton, mirroring the landed auth migration (#14836). - Public S3 APIs (copy, downloadData, getProperties, getUrl, list, remove, uploadData) gain (ctx, input) overloads with a global fallback via resolveCtxArgs - Internal workers, resolveS3ConfigAndInput, and access-grant internals take ctx: AmplifyContext; config via ctx.resourcesConfig and auth via ctx.fetchAuthSession - Server wrappers accept AmplifyContext | AmplifyServer.ContextSpec via new resolveServerContext, preserving adapter-nextjs compatibility - Tests migrated to a branded mock AmplifyContext (createMockAmplifyContext) Excludes the endpoint-provider feature (depends on unlanded core Storage types) and does not delete server impls (adapter-nextjs split not yet landed).
resolveServerContext previously cast the unwrapped AmplifyClass from getAmplifyServerContext(spec).amplify directly to AmplifyContext. But AmplifyClass only exposes resourcesConfig/libraryOptions fields and an Auth member -- it has no top-level fetchAuthSession/clearCredentials/ getTokens methods (those live on the branded context built in configure()). On the server path, resolveS3ConfigAndInput calls ctx.fetchAuthSession(), which threw at runtime. Adapt the AmplifyClass into a real AmplifyContext by bridging the context methods to amplify.Auth.*, and use the isAmplifyContext brand check instead of a structural 'resourcesConfig in x' probe (AmplifyClass also has resourcesConfig, so the probe was unsafe). Adds a guarding unit test and updates the six server-wrapper tests to a realistic AmplifyClass mock.
2b027b4 to
bf3cb37
Compare
| ctx: AmplifyContext, | ||
| ): Promise<ListPathsOutput> => { | ||
| const { buckets } = ctx.resourcesConfig.Storage!.S3!; | ||
| const { groups } = ctx.resourcesConfig.Auth!.Cognito; |
There was a problem hiding this comment.
Thanks for addressing the other five points, the live getters, JSDoc additions, explicit-context test coverage, and the resolveServerContext comment all look good now.
One item from the earlier round still seems open here. ctx.resourcesConfig.Storage!.S3! and ctx.resourcesConfig.Auth!.Cognito are still forced non-null assertions. Since listPaths now takes an explicit AmplifyContext instead of the global singleton, callers can more easily pass a context with a partial config (storage-only, no Auth, or Storage without an S3 sub-key). In that case this throws a bare TypeError: Cannot read properties of undefined instead of a descriptive StorageError.
Would you mind guarding these with an assertValidationError check (or optional chaining before reading .Cognito) so callers get an actionable StorageValidationErrorCode instead? Something like:
const { Storage, Auth } = ctx.resourcesConfig;
assertValidationError(!!Storage?.S3, StorageValidationErrorCode.NoS3Config);
assertValidationError(!!Auth?.Cognito, StorageValidationErrorCode.NoAuthConfig);
const { buckets } = Storage.S3;
const { groups } = Auth.Cognito;(exact error codes TBD, just illustrating the shape). Happy to discuss if there is a reason the non-null assertions are safe here that I am missing.
There was a problem hiding this comment.
Fair point — the explicit ctx does make partial configs newly reachable here, so I've pulled this into the PR after all. Done in 24e9ac2:
- Added
NoS3Config/NoAuthConfigtoStorageValidationErrorCodewith messages in the validation error map. listPathsnow guards both reads withassertValidationError(narrowing-friendly locals, no!assertions left), following theresolveS3ConfigAndInputprecedent.- Two new tests covering the missing-
Storage.S3and missing-Auth.Cognitocases.
osama-rizk
left a comment
There was a problem hiding this comment.
Careful, correctly-scoped migration that clears the bar that matters. I traced it end-to-end and verified the one thing that would silently break the server path — no call site still reaches the global singleton. Nothing blocking; findings below are mostly cleanups the diff surfaced, plus one error-surface change worth confirming.
What I verified
Migration completeness (the thing that kills a migration like this). A single missed call site — one worker still reaching Amplify.getConfig() / Amplify.Auth — compiles fine, passes most tests, and then silently uses the global config on the isolated server path (a cross-request credential leak in the worst case). So I enumerated all 154 storage src/*.ts files on the head branch and grepped for surviving global usage. The only hits are ctx.fetchAuthSession() / amplify.fetchAuthSession() (the correct threaded calls) and one comment. Zero global leakage. For a 64-file singleton removal, that's the result you want.
The server bridge is correct and complete. Checked resolveServerContext against the actual AmplifyContext interface: it supplies all five members (resourcesConfig getter, libraryOptions, fetchAuthSession, clearCredentials, getTokens) — no missing method that would throw at runtime. The fetchAuthSession: options => …(options ?? {}) default is right (context options optional, AuthClass's required); getTokens correctly omits the default. The test is genuinely adversarial — it mocks a method-less AmplifyClass, exactly the object a plain cast blows up on, and asserts the branded branch doesn't consult getAmplifyServerContext. That's the right way to test a bridge: reproduce the runtime shape the type system hides.
The disclosure that landed auth #14836 has the same latent server-path bug (getCurrentUser → ctx.getTokens on a method-less class) is exactly the note to surface. Endorse the follow-up — a shared core-level server bridge is the correct end state so each category doesn't re-implement this.
1. Unconfigured-path error surface changes (inherited from core, but Storage now exposes it)
Pre-migration, uploadData({path}) before Amplify.configure() flowed into resolveS3ConfigAndInput, where getConfig()?.Storage?.S3 ?? {} yielded no bucket → a StorageValidationErrorCode (NoBucket). Post-migration, resolveCtxArgs calls getGlobalContext(), which now throws 'No AmplifyContext available…' before any Storage validation runs. So a misconfigured/too-early call surfaces a different error name and message than on v6.
This originates in core (landed with auth), not this PR — but Storage's public APIs now inherit it, and anyone catching the old validation code on the unconfigured path sees a change. Worth a one-line acknowledgment that the pre-configure() error surface shifts, and confirmation that's intended for v7. Not blocking — it's a strictly clearer error, just a different one.
2. remove server wrapper: collapse the redundant if/else
In server/remove.ts both branches are now byte-identical:
if ('key' in input) {
return removeInternal(ctx, input);
} else {
return removeInternal(ctx, input);
}Pre-existing (each branch previously inlined getAmplifyServerContext(contextSpec).amplify), but since the PR rewrote exactly these lines, the dead discrimination should collapse to a single return removeInternal(ctx, input);. Cosmetic.
3. JSDoc description drift on the server @param
Across the server wrappers the param was renamed contextSpec → ctxOrContextSpec, but the description still reads "The isolated server context." It's now either an isolated server context or a direct AmplifyContext — the whole point of resolveServerContext. Update the description text (e.g. "The isolated server context, or a resolved AmplifyContext."), not just the name. Trivial.
4. (...args: any[]) impl erases internal type-checking (accepted tradeoff, worth naming)
The any[] impl means TS can't verify the body against the overloads — if resolveCtxArgs's returned tuple ever drifted from what the impl destructures, the compiler wouldn't catch it. This is the established auth-#14836 pattern and the mitigations are real (public overloads constrain callers; resolveCtxArgs<[Input]> is generically typed; tests exercise both arities). I'd accept it — flagging only that the safety net moved from the compiler onto resolveCtxArgs being correct, which is why core's resolveCtxArgs.test.ts matters as much as any test here.
Scope calls I agree with
- Endpoint-provider feature and server-impl deletion are correctly deferred (depend on unlanded core
Storage/types.ts+ adapter-nextjs split). - Required
ctx(no global fallback) oninternals/apis/*is the right call given first-party consumers migrate in lockstep on this branch.
Description
Migrates the
@aws-amplify/storagepackage from the globalAmplifysingleton to explicitAmplifyContextthreading, following the same pattern as the landed auth migration (#14836). This is the B-storage step of the v6→v7 context migration, targeting thefeat/bobbor/v6-contextfeature branch.What changed
copy,downloadData,getProperties,getUrl,list,remove,uploadData): added(ctx, input)overloads alongside(input), with a variadic impl usingresolveCtxArgs(global-context fallback preserved for existing callers).resolveS3ConfigAndInput, access-grant internals +listPaths: now takectx: AmplifyContextfirst; config viactx.resourcesConfig(notAmplify.getConfig()), auth viactx.fetchAuthSession().AmplifyContext | AmplifyServer.ContextSpecvia a newresolveServerContext, preserving adapter-nextjs compatibility. Server impl files are not deleted.AmplifyContext(createMockAmplifyContext); underlying modules mocked rather thanAmplify.getConfig.Deliberately out of scope (vs v7-poc)
endpointProvider/forcePathStyle) — depends on coreStorage/types.tschanges not yet on this base branch.resolveS3ConfigAndInput/base.tsretain the existingLOCAL_TESTING_S3_ENDPOINTlogic.Notable fix
resolveServerContextadapts the unwrappedAmplifyClass(fromgetAmplifyServerContext) into a realAmplifyContextby bridgingfetchAuthSession/clearCredentials/getTokenstoamplify.Auth.*. A bareAmplifyClasslacks those top-level methods, so a plain cast would throw on the server path at runtime. Uses theisAmplifyContextbrand check rather than a structural probe. A guarding unit test covers this.Testing
yarn build --scope @aws-amplify/storage→ passesyarn test --scope @aws-amplify/storage→ 86 suites / 858 tests pass, lint clean, ts-coverage passes;resolveServerContext.tsat 100% coverageChecklist
main)tsconfig.tsbuildinfocommittedNote:
internals/APIs take a requiredctx(no global fallback)Unlike the public S3 APIs (which keep the zero-ctx overload via
resolveCtxArgsfor backward compatibility), theinternals/apis/*functions re-exported from@aws-amplify/storage/internalsnow take a requiredAmplifyContextfirst parameter. This is intentional: this surface is consumed by first-party packages (StorageBrowser /ui-react-storage) that migrate in lockstep on thev6-contextfeature branch.